🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20662

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 Cancelled Job, 6 Pending, 1 Unrelated Failure

As of commit 074047e with merge base a6d812a ():

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@claude Review this PR.

Claude finished @digantdesai's task in 4m 23s —— View job

PR Review: Arm backend — Add Arm Neural Statistics

I reviewed the runtime C++ (VGFNeuralStatistics.{h,cpp}, VGFSetup.{h,cpp}, VGFBackend.cpp), the CMake integration, the Python Inspector parsing, docs, and tests.

Overall this is a well-structured, genuinely opt-in feature. The lifetime handling of the metadata string is correct (end_profiling_delegate copies into the flatbuffer immediately), memory ownership is clean (statistics memory is pushed into intermediates and freed once in free_vgf, no double-free), the failure paths degrade gracefully, and the base64/JSON serialization looks correct. Good test coverage of the serialization and the test-collector hook.

A few things worth addressing before merge:

1. Does actually capturing statistics require a session-creation flag? (main question)

In VGFSetup.cpp:2978, the session is always created with .flags = 0, and the neural-statistics bind point is only handled if the driver happens to advertise it. For many VK_ARM_data_graph implementations the neural-accelerator statistics bind point is only advertised when the pipeline/session is created requesting statistics capture. If that's the case here, EXECUTORCH_VGF_ENABLE_NEURAL_STATISTICS=1 would still produce data_available: false on real hardware because nothing ever asks the driver to produce the data. Can you confirm whether the current path yields real statistics on a supported driver, and if a creation-time flag is needed, whether it should be set (gated by the env var)?

2. Statistics bind-point memory is allocated even when the feature is disabled

The env var (vgf_neural_statistics_profiling_enabled()) only gates collection/serialization in VGFBackend::execute (VGFBackend.cpp:382). The bind-point discovery and vkAllocateMemory/bind in VGFSetup.cpp:3021-3181 run unconditionally at setup whenever the bind point is advertised. This slightly contradicts the "normal timing-only profiling unchanged by default" claim — if the driver advertises the bind point, you allocate and bind extra device memory regardless of the env var. Consider skipping the neural-statistics bind-point allocation entirely (marking it unavailable with a reason) when the feature is off. Fix this →

3. Redundant available field + misleading comment

In VGFNeuralStatistics.cpp:453-455:

// Write that not available
out << ",\"available\":";
append_bool(out, collection.data_available);

available duplicates data_available exactly, and the comment ("Write that not available") is a leftover that doesn't describe what the code does. Either drop the duplicate field or document why both exist for consumer convenience, and fix/remove the comment.

4. Env-var parsing is case-inconsistent

vgf_neural_statistics_profiling_enabled() (VGFBackend.cpp:107) treats "0", "false", "FALSE", "off", "OFF" as disabled but not "False"/"Off"/"no". Minor, but since it's documented only as =1, consider simplifying to "any non-empty value other than 0/false/off (case-insensitive)" or just check for 1/true.

5. Minor

• VgfNeuralStatisticsSegmentContext::session (VGFNeuralStatistics.h:50) is populated in get_neural_statistics_segment_contexts() but never read anywhere in collection (queries use pipeline). Dead field — drop it unless it's intended for a future version.
• append_blob runs json_escape over the (potentially large) base64 string (VGFNeuralStatistics.cpp:116). base64 output contains no characters that need escaping, so this is wasted work on large blobs; wrapping in quotes directly would be cheaper. Cosmetic.
• The new doc docs/source/backends/arm-vgf/vgf-neural-statistics.md doesn't appear to be linked from a toctree/index — confirm it's discoverable in the built docs.

CI

The one new failure is test-qnn-python-imports-linux. This PR eagerly imports vgf_neural_statistics from devtools/inspector/__init__.py, but that module only pulls in stdlib (base64, binascii, json, typing), so it's unlikely to be the cause. Worth confirming the failure is unrelated/flaky rather than an import-ordering regression, since __init__.py now imports at package-load time (the _inspector.py accessor correctly imports lazily).

None of these are blockers except possibly #1 if it means the feature can't produce data as-is. Nice work overall.
· branch vgf_database